Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace snafu with thiserror #93

Closed

Conversation

elpiel
Copy link

@elpiel elpiel commented Oct 27, 2020

Issue #70
From what I see the difference between the two is not that big. If no_std has more priority I would suggest to leave snafu for the time being.

WrongEncoding {},
#[snafu(display("{}", source))]
#[snafu(context(false))]
#[error(transparent)]
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will handle the impl of Display to the underlying error.

@@ -23,11 +23,6 @@
//! * [cloudevents-sdk-reqwest](https://docs.rs/cloudevents-sdk-reqwest): Integration with [reqwest](https://github.com/seanmonstar/reqwest)
//!

extern crate serde;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for extern crate in the 2018 edition

@elpiel elpiel force-pushed the replace-snafu-with-thiserror branch 2 times, most recently from 245fa16 to ab2008d Compare October 27, 2020 11:15
@elpiel
Copy link
Author

elpiel commented Oct 27, 2020

I'm also getting an unused import warning, maybe it's good to fix it as well?

warning: unused import: `attributes::AttributesIter`
  --> src/event/mod.rs:13:16
   |
13 | pub(crate) use attributes::AttributesIter;
   |                ^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(unused_imports)]` on by default

@slinkydeveloper
Copy link
Member

@elpiel this should be fixed in another pr. let me merge them and rebase this one on top of master

@slinkydeveloper
Copy link
Member

@elpiel can you rebase?

@slinkydeveloper slinkydeveloper added the enhancement New feature or request label Oct 28, 2020
@elpiel elpiel force-pushed the replace-snafu-with-thiserror branch from ab2008d to eba7ee9 Compare October 28, 2020 12:32
@elpiel
Copy link
Author

elpiel commented Oct 28, 2020

Should be done.

@pranav-bhatt
Copy link
Contributor

@elpiel I'm currently working on making cloudevents-sdk-rust no_std compatible here #94. Since you have worked with thiserror, could we implement it in such a way that we can satisfy this need as well? We could continue this discussion in that PR ig.

@slinkydeveloper
Copy link
Member

Since it seems the no_std work is in progress, I'm gonna close this PR now (and i'm gonna close the issue too). @elpiel feel free to join in our cncf slack channel https://cloud-native.slack.com/archives/CTS0TCGVA to chat a little bit more about that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants